Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Rename OOM to Watchdog Terminations #2499

Merged
merged 9 commits into from
Dec 12, 2022
Merged

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Dec 6, 2022

Rename OOM to Watchdog Terminations. Basically a pretty simple search and replace without any functional changes.

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1239.54 ms 1277.42 ms 37.88 ms
Size 20.75 KiB 404.99 KiB 384.24 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
c6fb5a9 1194.80 ms 1232.68 ms 37.88 ms
010583c 1198.23 ms 1225.52 ms 27.29 ms
a9e77dc 1231.94 ms 1254.85 ms 22.91 ms
f91714d 1222.06 ms 1247.00 ms 24.94 ms
d446105 1237.06 ms 1261.34 ms 24.28 ms
2ae7db9 1231.37 ms 1239.98 ms 8.61 ms
ffa889e 1229.37 ms 1253.38 ms 24.01 ms
8361c4c 1204.07 ms 1252.74 ms 48.67 ms
4dc66f6 1202.59 ms 1228.34 ms 25.75 ms
9be1db2 1219.42 ms 1245.66 ms 26.24 ms

App size

Revision Plain With Sentry Diff
c6fb5a9 20.75 KiB 383.76 KiB 363.01 KiB
010583c 20.75 KiB 383.61 KiB 362.85 KiB
a9e77dc 20.75 KiB 379.12 KiB 358.36 KiB
f91714d 20.75 KiB 381.87 KiB 361.12 KiB
d446105 20.75 KiB 383.37 KiB 362.62 KiB
2ae7db9 20.75 KiB 381.87 KiB 361.12 KiB
ffa889e 20.75 KiB 383.83 KiB 363.08 KiB
8361c4c 20.75 KiB 383.87 KiB 363.12 KiB
4dc66f6 20.75 KiB 381.81 KiB 361.06 KiB
9be1db2 20.75 KiB 373.94 KiB 353.19 KiB

Previous results on branch: ref/2399-rename-OOM

Startup times

Revision Plain With Sentry Diff
1d9116e 1249.71 ms 1257.94 ms 8.22 ms

App size

Revision Plain With Sentry Diff
1d9116e 20.75 KiB 404.99 KiB 384.23 KiB

* 8.0.0:
  chore: Update homebrew bundle (#2495)
  ci: Fix Slather for Codecov (#2494)
  feat: Enable File I/O APM by default (#2497)
@kevinrenskers kevinrenskers marked this pull request as ready for review December 6, 2022 14:42
*/
@property (nonatomic, assign) BOOL enableOutOfMemoryTracking;
@property (nonatomic, assign) BOOL enableWatchdogTerminationsTracking;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enableWatchdogTerminationsTracking or enableWatchdogTerminationTracking, not 100% sure which is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer enableWatchdogTerminationTracking

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have also preferred enableWatchdogTerminationTracking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is now open with that change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @kevinrenskers 😀

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #2499 (302966b) into 8.0.0 (6bb5919) will decrease coverage by 0.02%.
The diff coverage is 94.44%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            8.0.0    #2499      +/-   ##
==========================================
- Coverage   78.44%   78.42%   -0.03%     
==========================================
  Files         242      241       -1     
  Lines       22282    22275       -7     
  Branches     9839     9835       -4     
==========================================
- Hits        17479    17469      -10     
- Misses       4351     4354       +3     
  Partials      452      452              
Impacted Files Coverage Δ
...s/Sentry/SentryWatchdogTerminationsScopeObserver.m 70.51% <ø> (ø)
Sources/Sentry/SentryBaseIntegration.m 90.90% <33.33%> (ø)
Sources/Sentry/SentryClient.m 95.78% <100.00%> (ø)
Sources/Sentry/SentryCrashIntegration.m 98.66% <100.00%> (ø)
Sources/Sentry/SentryOptions.m 87.20% <100.00%> (ø)
Sources/Sentry/SentrySessionCrashedHandler.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryWatchdogTerminationsLogic.m 62.26% <100.00%> (ø)
Sources/Sentry/SentryWatchdogTerminationsTracker.m 100.00% <100.00%> (ø)
...ry/SentryWatchdogTerminationsTrackingIntegration.m 86.36% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bb5919...302966b. Read the comment docs.

@kevinrenskers kevinrenskers changed the title Rename OOM to Watchdog Terminations red: Rename OOM to Watchdog Terminations Dec 6, 2022
@kevinrenskers kevinrenskers changed the title red: Rename OOM to Watchdog Terminations ref: Rename OOM to Watchdog Terminations Dec 6, 2022
@kevinrenskers
Copy link
Contributor Author

The codecov step is resulting in a red cross: 78.42% (-0.03%) compared to 6bb5919.

Is that really worth an error? 🤔

@kevinrenskers
Copy link
Contributor Author

Note to self: sentry-docs needs to be updated.

@kevinrenskers
Copy link
Contributor Author

Docs PR: getsentry/sentry-docs#5938. I think we can merge this one? Waiting for approval.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Just wait @philipphofmann to vote for the name

@kevinrenskers
Copy link
Contributor Author

I am going to merge this now since it's been waiting for six days and I want to prevent merge conflicts since this touches a bunch of stuff. If we want to change the option name, I can follow up with another PR.

@kevinrenskers kevinrenskers merged commit ba46ce3 into 8.0.0 Dec 12, 2022
@kevinrenskers kevinrenskers deleted the ref/2399-rename-OOM branch December 12, 2022 09:35
kevinrenskers added a commit that referenced this pull request Dec 16, 2022
* 8.0.0: (31 commits)
  tests: Reenable testAddAndRemoveData (#2533)
  feat: support SENTRY_DSN environment var on macOS (#2534)
  Remove duplicate entry (#2532)
  fix: ARC issue for FileManager (#2525)
  feat: Add SwiftUI performance tracking (#2271)
  fix: Remove all permission checks (#2529)
  Remove the automatic `viewAppearing` span (#2511)
  Fix and reenable testANRDetected_UpdatesAppStateToTrue (#2526)
  fix: Don't add out of date context for crashes (#2523)
  ref: Rename isOOM to watchdog in Client (#2520)
  test: Fix disabled failing watchdog test (#2521)
  build(deps): bump github/codeql-action from 2.1.35 to 2.1.36 (#2516)
  Rename the watchdog option and integration (#2513)
  feat: Enable CaptureFailedRequests by default (#2507)
  test: Fix asserts for SentryCrashTestInstallation (#2500)
  meta: User interaction tracing enabled per default (#2506)
  ref: Rename OOM to Watchdog Terminations (#2499)
  feat: enableUserInteractionTracing is GA (#2503)
  build(deps): bump nokogiri from 1.13.9 to 1.13.10 (#2505)
  perf: Don't attach headers for SentryNoOpSpan (#2498)
  ...
philipphofmann added a commit to getsentry/sentry-docs that referenced this pull request Jan 10, 2023
OOM is now called watchdog terminations. See getsentry/sentry-cocoa#2499, which will be part of the 8.0.0 release of sentry-cocoa.

Co-authored-by: Isabel <[email protected]>
Co-authored-by: Philipp Hofmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants